feat($interpolate): escape interpolated expressions #5628
Conversation
| @@ -236,6 +268,11 @@ function $InterpolateProvider() { | |||
| return endSymbol; | |||
| }; | |||
|
|
|||
|
|
|||
| function unescape(text) { | |||
There was a problem hiding this comment.
Would this need to be documented? As an internal function, given the name its use seems pretty clear
|
I have some doubts that the patch as-is works as expected in the real world when the |
|
You mean in conjunction with the HTML compiler, yes it starts to get tricky there. Write some tests and try to break it, for sure. We have a few weeks before this is even considered for merging in. |
|
Okay, this has finally been rebased and the bitrot is fixed! The changes to $interpolate made it a bit harder to make this work, and it might kill all of the performance benefits that you'd otherwise get =( The escaping algorithm has been made more complicated now out of necessity, so I think it's likely broken in some cases, and probably needs some more robust testing. @lgalfaso, @shahata, @btford, whoever else, PTAL and see if you can break it when you have a chance, alright? |
By default, the escaped interpolation start and end symbols are `{{{{` and
`}}}}`, respectively.
These symbols will be replaced with the proper interpolation start and end
symbols during interpolation, without parsing the expression.
Fixes angular#5601
|
Sorry, I had to read the entire #5601 before understanding the why of this PR. Truth be told, the more I read about this issue, the more I think this PR is an overkill. If the idea is to be able to handle <%= user.profile %>in a safe way, then there is a much simpler solution to this. Create a function (say
Then write the template as {{'<%= ngEscape(user.profile) %>'}}Just to be clear, there might be cases where there might be a need for |
|
I don't think so @lgalfaso --- The issue is that we're trying to avoid script injection, and anything within |
|
@caitp let me read the original issue once again and then will go to the PR As a side note the idea was to create a javascript string with |
|
The idea is that the web application can upload arbitrary data, which later an be rendered by angular, thus allowing a form of script injection. The server can easily escape these by replacing double curlies with quadruple-curlies (for example), thus mitigating problems and showing the expected text. Otherwise angular throws and renders nothing |
|
@caitp I am still trying to understand some of the cases, is the idea that the server will replace expect($interpolate('{{{{{{{{abcdef}}}}}}')($rootScope)).toBe('{{{{abcdef}}}}');
expect($interpolate('{{{{{{{{abcdef}}}}}}}}')($rootScope)).toBe('{{{{abcdef}}}}');correct? |
|
I dunno, I am exhausted and will have to look at it again tomorrow |
| * @returns {string|self} Returns the symbol when used as getter and self if used as setter. | ||
| */ | ||
| this.endSymbol = function(value){ | ||
| this.endSymbol = function(value, escaped){ |
There was a problem hiding this comment.
I think you should add unit tests for setting startSymbol and endSymbol with escaped
|
@caitp I think that the main problem with this PR is that you are expecting the escaped interpolation symbol (aka quadruple-curlies) to end at some point, assuming the attacker was nice enough to close them. I mean, if for example I have something like this in my server side template: <div><%= unsafeVariable %> {{abc}}</div>An attacker can put something like Actually, I believe that this code can be much simpler if we do not expect quadruple-curlies to end and just take them in as regular string and replace them back to double-curlies |
| exp = text.substring(startIndex + startSymbolLength, endIndex); | ||
| expressions.push(exp); | ||
| parseFns.push($parse(exp)); | ||
| index = endIndex + endSymbolLength; | ||
| hasInterpolation = true; | ||
| i = -1; | ||
| } else if (escaped && startIndex >= 0 && escapedStart < (endIndex + endSymbolLength)) { |
There was a problem hiding this comment.
This is a bit confusing. Could be just else if you move the above (!escaped || escapedEnd < startIndex || escapedStart > endIndex + endSymbolLength) into a nested condition.
|
it seems that most of the complexity in this change is coming from the requirement that the interpolation markers might be a substring of the escaped markers. Is that right? If that's the case, then wouldn't things be significantly easier if we said that that can't be the case? Once the interpolation and escaped markers are easy to distinguish then we could run interpolation as is today and then run through all separators and see if we can find the escaped markers there and replace them with interpolation markers. |
|
@IgorMinar I rewrote this using (non-greedy) regular expressions instead. The CL is still a bit complicated, but it removes all of the hacks, pretty much. It would be better to criticize the updated one |
|
this got implemented as e3f78c1 |
By default, the escaped interpolation start and end symbols are
{{{{and}}}}, respectively.These symbols will be replaced with the proper interpolation start and end symbols during interpolation, without parsing the expression.
Fixes #5601
These tests might be made better, and the implementation might be improved. Comments welcome :3